rustc_span: Generate keyword classification functions automatically #75309

petrochenkov · 2020-08-08T21:58:01Z

The generated functions are compatible with symbols not forming continuous ranges.
This is a nicer alternative to 4896614 from #74554.
r? @nnethercote

petrochenkov · 2020-08-08T22:04:43Z

@bors try @rust-timer queue

The classification functions are array searches rather than simpler range checks, so they are supposed to be slower.
However, parsing is fast in general, so this shouldn't have a noticeable effect?

For small lengths (like those of the keyword lists) linear search is preferable to binary search.
For optimal speed the arrays should be sorted (to please branch predictor), but this would require copypasting some logic from rustc_span to rustc_macros after #74554 is merged.

rust-timer · 2020-08-08T22:04:44Z

Awaiting bors try build completion

bors · 2020-08-08T22:04:55Z

⌛ Trying commit 260234aa5f640d8f080e84a58d9292900b3f7348 with merge 2e0259109d9ebdf5660e31aa7bce983d3108f27c...

Mark-Simulacrum · 2020-08-08T22:16:58Z

src/librustc_macros/src/symbols.rs

+ if let Some(class) = class {
+ keyword_class_stream.extend(quote! {
+ fn #class(self) -> bool {
+ [#class_stream].contains(&self)


It does seem a bit unfortunate to lose the O(1) range-checking here, but I guess this is probably not too hot.

Maybe LLVM takes care of things for us, too, though I somewhat doubt it.

Edit: Oh, I see your comment now. We can wait on perf.

I was expecting that we'd lower to #class_stream[0] <= self && self <= #class_stream.last, though obviously that syntax isn't actually a thing.

This PR is supposed to land only together with #74554, which also regresses performance due to inability to do range checks, but improves it more due to inlining.

bors · 2020-08-08T22:44:51Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 2e0259109d9ebdf5660e31aa7bce983d3108f27c (2e0259109d9ebdf5660e31aa7bce983d3108f27c)

rust-timer · 2020-08-08T22:44:53Z

Queued 2e0259109d9ebdf5660e31aa7bce983d3108f27c with parent 1facd4a, future comparison URL.

rust-timer · 2020-08-09T00:01:50Z

Finished benchmarking try commit (2e0259109d9ebdf5660e31aa7bce983d3108f27c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

Mark-Simulacrum · 2020-08-09T00:19:10Z

Seems like a definite slow down, but mostly only affecting doc builds (1-2% on smaller crates). <1% regression in instruction counts on non-doc builds.

nnethercote · 2020-08-09T02:02:22Z

Yeah, maybe we shouldn't land this until we are certain about #74554?

petrochenkov · 2020-08-09T08:57:47Z

src/librustc_macros/src/symbols.rs

+ let mut current_class = None;
+ while !input.is_empty() {
+ if input.peek(Token![fn]) {
+ input.parse::<TokenTree>()?;


I wonder if there's any simpler way to skip a token with syn parser.

petrochenkov · 2020-08-09T09:18:17Z

@bors try @rust-timer queue

rust-timer · 2020-08-09T09:18:19Z

Awaiting bors try build completion

bors · 2020-08-09T09:18:28Z

⌛ Trying commit 1003529c05c7e35faea2e246758d77b7151853ce with merge a69ffd7a844a5bb0dea3220cceeb958ab7a2d3ad...

bors · 2020-08-09T10:02:35Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: a69ffd7a844a5bb0dea3220cceeb958ab7a2d3ad (a69ffd7a844a5bb0dea3220cceeb958ab7a2d3ad)

rust-timer · 2020-08-09T10:02:37Z

Queued a69ffd7a844a5bb0dea3220cceeb958ab7a2d3ad with parent f50f1c8, future comparison URL.

rust-timer · 2020-08-09T11:19:47Z

Finished benchmarking try commit (a69ffd7a844a5bb0dea3220cceeb958ab7a2d3ad): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

petrochenkov · 2020-08-09T11:26:22Z

Ok, match looks better than contains (but is still a regression).

petrochenkov · 2020-08-09T11:58:06Z

src/librustc_span/symbol.rs

@@ -26,12 +26,14 @@ symbols! {
 Keywords {
 // Special reserved identifiers used internally for elided lifetimes,
 // unnamed method parameters, crate root module, error recovery etc.
+ fn is_special:


This confuses code highlighting in my editor (vscode + rust-analyzer), but fn is required for searchability.
Perhaps the list needs to go into braces, e.g. like this:

fn is_special { Invalid: "", PathRoot: "{{root}}", DollarCrate: "$crate", Underscore: "_", }

petrochenkov · 2020-08-11T20:26:55Z

error[E0107]: wrong number of type arguments: expected 3, found 2
  --> src/librustc_macros/src/symbols.rs:50:17
   |
50 | struct Keywords(IndexMap<Option<Ident>, Vec<Keyword>>);
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 3 type arguments

Excuse me?
Somehow rustc_macros gets built without has_std feature being enabled for the indexmap crate.

Mark-Simulacrum · 2020-08-11T20:45:32Z

Hm so indexmap is using autocfg to probe for std's presence, I think via this bit of code. My guess is that's not using the right rustc or not passing it the right parameters, but I'm not sure without seeing the actual build log from autocfg we'd be able to say much.

The check in rustdoc using it is artificial and not helpful

petrochenkov · 2020-08-15T19:27:04Z

I couldn't reproduce the indexmap error locally, but fixed it by providing the hasher argument explicitly.

shepmaster · 2020-08-24T19:11:51Z

My guess is that's not using the right rustc or not passing it the right parameters

See also rust-lang/cargo#8646

petrochenkov · 2020-08-26T21:15:12Z

So, this PR should probably just be cherry-picked into #74554 since it's not valuable by itself.

bors · 2020-08-30T17:56:14Z

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

petrochenkov · 2020-08-31T20:20:18Z

Closing since #74554 has been closed.

rust-highfive assigned nnethercote Aug 8, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 8, 2020

petrochenkov mentioned this pull request Aug 8, 2020

Inlined symbols #74554

Closed

Mark-Simulacrum reviewed Aug 8, 2020

View reviewed changes

petrochenkov commented Aug 9, 2020

View reviewed changes

petrochenkov force-pushed the classym branch from 260234a to 1003529 Compare August 9, 2020 09:17

petrochenkov added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 9, 2020

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 9, 2020

petrochenkov commented Aug 9, 2020

View reviewed changes

petrochenkov force-pushed the classym branch from 1003529 to e006703 Compare August 11, 2020 19:57

petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 11, 2020

petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2020

rustc_span: Remove Symbol::is_doc_keyword

e37b75e

The check in rustdoc using it is artificial and not helpful

petrochenkov force-pushed the classym branch from e006703 to 6cfadba Compare August 15, 2020 19:24

rustc_span: Generate keyword classification functions automatically

f02b037

petrochenkov force-pushed the classym branch from 6cfadba to f02b037 Compare August 15, 2020 19:43

petrochenkov closed this Aug 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc_span: Generate keyword classification functions automatically #75309

rustc_span: Generate keyword classification functions automatically #75309

petrochenkov commented Aug 8, 2020

petrochenkov commented Aug 8, 2020

rust-timer commented Aug 8, 2020

bors commented Aug 8, 2020

Mark-Simulacrum Aug 8, 2020 •

edited

Loading

petrochenkov Aug 9, 2020

bors commented Aug 8, 2020

rust-timer commented Aug 8, 2020

rust-timer commented Aug 9, 2020

Mark-Simulacrum commented Aug 9, 2020

nnethercote commented Aug 9, 2020

petrochenkov Aug 9, 2020

petrochenkov commented Aug 9, 2020

rust-timer commented Aug 9, 2020

bors commented Aug 9, 2020

bors commented Aug 9, 2020

rust-timer commented Aug 9, 2020

rust-timer commented Aug 9, 2020

petrochenkov commented Aug 9, 2020 •

edited

Loading

petrochenkov Aug 9, 2020

petrochenkov commented Aug 11, 2020

Mark-Simulacrum commented Aug 11, 2020

petrochenkov commented Aug 15, 2020

shepmaster commented Aug 24, 2020

petrochenkov commented Aug 26, 2020

bors commented Aug 30, 2020

petrochenkov commented Aug 31, 2020

rustc_span: Generate keyword classification functions automatically #75309

rustc_span: Generate keyword classification functions automatically #75309

Conversation

petrochenkov commented Aug 8, 2020

petrochenkov commented Aug 8, 2020

rust-timer commented Aug 8, 2020

bors commented Aug 8, 2020

Mark-Simulacrum Aug 8, 2020 • edited Loading

Choose a reason for hiding this comment

petrochenkov Aug 9, 2020

Choose a reason for hiding this comment

bors commented Aug 8, 2020

rust-timer commented Aug 8, 2020

rust-timer commented Aug 9, 2020

Mark-Simulacrum commented Aug 9, 2020

nnethercote commented Aug 9, 2020

petrochenkov Aug 9, 2020

Choose a reason for hiding this comment

petrochenkov commented Aug 9, 2020

rust-timer commented Aug 9, 2020

bors commented Aug 9, 2020

bors commented Aug 9, 2020

rust-timer commented Aug 9, 2020

rust-timer commented Aug 9, 2020

petrochenkov commented Aug 9, 2020 • edited Loading

petrochenkov Aug 9, 2020

Choose a reason for hiding this comment

petrochenkov commented Aug 11, 2020

Mark-Simulacrum commented Aug 11, 2020

petrochenkov commented Aug 15, 2020

shepmaster commented Aug 24, 2020

petrochenkov commented Aug 26, 2020

bors commented Aug 30, 2020

petrochenkov commented Aug 31, 2020

Mark-Simulacrum Aug 8, 2020 •

edited

Loading

petrochenkov commented Aug 9, 2020 •

edited

Loading